-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Crypto import for non-node envs #1307
Conversation
…nd acted on. add more authz grant queries to catch up to current authz module
Thank you. This looks both promising at first glance. Could you split the additional queries and the crypto changes in 2 PRs please? This way we can keep the discussion specific and not block one change on the other change. |
Done, the authz queries PR is now in #1308 |
@@ -11,9 +11,9 @@ import { sha512 as nobleSha512 } from "@noble/hashes/sha512"; | |||
*/ | |||
export async function getCryptoModule(): Promise<any | undefined> { | |||
try { | |||
const crypto = await import("crypto"); | |||
const crypto = await require("crypto"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try this instead?
// HACK: Use a variable to get webpack to ignore this and cause a
// runtime error instead of build system error or fallback implementation.
const nodeCryptoPackageName = "crypto";
const crypto = await import(nodeCryptoPackageName);
We use this in a different place already. This should avoid build systems trying to bundle a crypto implementation when it is not available natively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webmaster128 I introduced this, however the problem is the same. It attempts to resolve crypto
when bundling. In my current project, using Expo/React-Native/Metro, it seems to parse/resolve any import
call, hence why I switched it to a require
. I believe this is specific to the Metro bundler.
Any ideas on how to solve this?
The change LGTM. Good point regarding the subtle import. I'd just like to get rid of the Node.js specific |
Sorry for letting you wait so long. I'm now trying to catch up with CosmJS. It seems like metro allows us to use an "empty" crypto module. See
I never worked with metro though. Do you have a test project where I can reproduce the issues you are facing? |
I think the motivation for the current codebase was this: subtle is either
But turns out there is a 3rd option in between:
I'll update the code to better reflect that. |
I looked even more into this and have two changes in the pipeline:
|
okay, this is far from solved. See #1354 for a follow-up ticket. |
crypto import:
change import pattern of crypto, to allow import error to be caught and acted on. the current model fails in react native, not exactly sure why, but using require instead avoids the issue.
Changed the lookup for
subtle
. Currently, ifsubtle
doesn't exist on the crypto library, cosmjs will attempt to reimport node crypto, even if a proper crypto was already shimmed in. The changeset will pull subtle if it exists, otherwise return undefined.getCryptoModule
will only be called ifcrypto
is not already set on window/thisauthz queries:
add more authz grant queries to catch up to current authz types